Passkey local fix and improvements #8185
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several fixes and improvements for passkey authentication. The most significant change is the removal of server-side configuration for passkey user verification, simplifying the logic to rely solely on client-side settings. This is a major refactoring that removes a considerable amount of code. Additionally, the pull request includes several important bug fixes, such as preventing a potential hang in the PAM responder, fixing a buffer over-read in authtok handling, and avoiding unnecessary PIN prompts. The changes are well-reasoned and improve the robustness of the passkey authentication feature. I have not found any high or critical severity issues in this pull request.
justin-stephenson
added
backport-to-sssd-2-8
backport-to-sssd-2-9
backport-to-sssd-2-10
backport-to-sssd-2-9-4
|
Please remove |
|
A set of 'backport-to' labels is questionable, imo. |
justin-stephenson
removed
backport-to-sssd-2-8
backport-to-sssd-2-9-4
justin-stephenson
force-pushed
the
passkey_misc_improvements
branch
from
c838a72 to
028f657
Compare
Done. |
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I have one question/concern with commit passkey: Remove SYSDB_PASSKEY_USER_VERIFICATION. In the passkey kerberos case, the user-verification is still used but instead of reading it from the IPA server we now read it from ipa-otpd. Is my understanding correct?
That's correct. It is sent to the SSSD krb5 passkey plugin from ipa-otpd https://github.com/freeipa/freeipa/blob/master/daemons/ipa-otpd/passkey.c#L43 |
justin-stephenson
force-pushed
the
passkey_misc_improvements
branch
from
028f657 to
c06768a
Compare
justin-stephenson
force-pushed
the
passkey_misc_improvements
branch
from
c06768a to
7a46611
Compare
|
I rebased to include #8212 - it was a clean rebase. |
alexey-tikhonov
added
no-backport
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
LGTM! Thank you for fixing this
|
@alexey-tikhonov why did you remove sssd-2-9 backport label? This is a fix for passkey issue |
There would be a conflict in sssd-2-9 because #8212 was merged only in 'master'. |
|
Note: Covscan is green. |
Remove support of ambiguous "unset" state of passkey user verification. pam_sss prompting is binary, either on or off. The use of 'unset' passkey user verification state allows for ambiguous behavior in SSSD. For example, passkey_child may perform undefined behavior when '--user-verification' argument is not set, now SSSD will always send '--user-verification=false/true' to passkey_child. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Local auth functions should only be reached in AD/LDAP auth flows. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Remove SYSDB_PASSKEY_USER_VERIFICATION and related functions. In phase 1 of passkey implementation we read passkey user verification from IPA LDAP tree, however now user verification is sent to the SSSD krb5 plugin from ipa-otpd. Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
Reviewed-by: Iker Pedrosa <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>
sssd-bot
force-pushed
the
passkey_misc_improvements
branch
from
7a46611 to
9a1841e
Compare
5 participants
This fixes local passkey auth (LDAP/AD) on the command-line when no PIN is set, it also addresses some improvements and small fixes that were found during recent passkey testing